Repository navigation
Conversation
…eouts-mixed Every delay in this upstream test is a ratio of headersTimeout, so halving the base from platformTimeout(2000) to platformTimeout(1000) preserves the relative schedule and all margins while halving wall time. The tightest window (request2's 408 arriving before the 1.4x checkpoint) is 0.075 * headersTimeout; platformTimeout keeps the debug doubling so that stays >= 150 ms on debug/ASAN builds. debug+ASAN, 30/30 pass idle, 10/10 under 8- and 14-way CPU load. bun bd <file>: 16.3s -> 10.8s.
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 9:46 AM PT - Jul 31st, 2026
✅ @robobun, your commit 71f6793c02c1571963edd49e872f73cdb68da95a passed in 🧪 To try this PR locally: bunx bun-pr 36563That installs a local version of the PR into your bun-36563 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only timing compression that preserves all relative margins.
What was reviewed:
- Traced every
setTimeoutmultiplier (0.2x/0.6x/0.8x/1.4x/1.5x/3.125x) andconnectionsCheckingInterval— all scale offheadersTimeout, so halving the base preserves every ratio. - Verified the narrowest-window claim: request2's 408 lands by 1.2x + 0.125x = 1.325x vs the 1.4x checkpoint → 0.075x slack, i.e. 75 ms release / 150 ms debug via
platformTimeout. Other windows (request1 headers 0.2x, request3/5 requestTimeout 0.4x+) are wider. - Confirmed no assertions/mustCall counts changed and the deviation-from-upstream comment matches repo convention.
Extended reasoning...
Overview
Single-constant change in a vendored Node.js sequential test: common.platformTimeout(2000) → common.platformTimeout(1000), plus a comment documenting the deviation from upstream and halved entries in test/expected-durations.json. No production code touched.
Security risks
None — test-only, no user-facing surface, no untrusted input.
Level of scrutiny
Low. This is a mechanical test-speed tweak following the same pattern as #33622 (test-performance-eventloopdelay.js). The only real risk is introducing flakiness by shrinking a timing window too far; the PR description addresses that head-on with 30/30 idle + 10/10 under-load runs and shows that lower candidates (800 ms fixed, 640 ms) do flake, which bounds the choice.
Other factors
I independently re-derived the timing schedule: request2 is opened at 0.2x, never sends the header terminator, so headersTimeout expires at 1.2x and the connections check (interval = 0.125x) closes it by 1.325x — leaving 0.075x before the 1.4x assertion. That matches the PR's narrowest-window analysis exactly. All other request paths (request1 completing headers at 0.8x before its 1.0x deadline; request3/4/5 requestTimeout deadlines vs the 3.125x final check) have ≥0.2x slack, so request2 is indeed the binding constraint. Retaining platformTimeout() keeps the debug/ASAN doubling, which is what makes the 150 ms figure hold on the slow lanes. The expected-durations.json edit is advisory bin-packing data that gets regenerated. The added comment is exactly the kind of "deliberate deviation from upstream" note REVIEW.md asks for.
…ation estimate Self-review caught that the CI asan lane builds with --profile=ci-* (buildType Release), so process.features.debug is false there and platformTimeout does not double. Reworded the in-file comment to state the 75 ms release window and the 150 ms debug-build window separately, and corrected the asan expected-durations estimate (the ~6 s LSAN/validateExceptionChecks teardown on that lane is not halved by the schedule change). Verified on a local release+ASAN build with the same env the runner sets (BUN_JSC_validateExceptionChecks=1, LSAN, BUN_DESTRUCT_VM_ON_EXIT, BUN_FEATURE_FLAG_NO_ORPHANS): 30/30 idle and 20/20 under 8-way CPU load at the new value.
There was a problem hiding this comment.
LGTM — test-only timing constant change, halving the base headersTimeout in a vendored Node sequential test.
What was reviewed:
- Traced all six
setTimeoutcheckpoints (0.2x/0.6x/0.8x/1.4x/1.5x/3.125x) — every margin scales proportionally; narrowest window is request2's 408 before the 1.4x assert at 0.075x (75 ms release), matching the comment. - Confirmed
platformTimeoutincommon/index.js:381doubles onisDebug, so the comment's release-ASAN vs local-debug distinction is accurate. expected-durations.jsonis advisory shard-packing metadata; values are plausible and regenerated periodically anyway.
Extended reasoning...
Overview
This PR touches two test-infrastructure files: it halves the headersTimeout base constant in test/js/node/test/sequential/test-http-server-request-timeouts-mixed.js from common.platformTimeout(2000) to common.platformTimeout(1000), adds a comment documenting the deviation from upstream Node and the narrowest timing window, and updates the corresponding entry in test/expected-durations.json to reflect the reduced wall-clock cost for CI shard bin-packing.
Security risks
None. No production code is touched — this is a vendored Node.js test file and a CI metadata JSON file.
Level of scrutiny
Low-to-moderate. The only real risk is introducing flakiness by shrinking timing windows too far. I re-derived the timing math for all five requests: request1 completes at 0.8x (0.6x margin before the 1.4x assert), request2's 408 must arrive by 1.4x after firing at worst 1.325x (0.075x = 75 ms release / 150 ms debug — the narrowest window, as stated), request3/5 time out at ~2.325x/2.725x with the final assert at 3.125x (≥0.4x margin), and request4 completes at 1.5x well before its 2.6x requestTimeout. All margins remain proportional because connectionsCheckingInterval = headersTimeout / 8 scales with the base. The PR description reports 30/30 idle passes plus 10/10 under 8-way and 14-way CPU load, and explicitly rejected lower values (800 ms, 640 ms) that flaked — exactly the empirical validation the review guidelines ask for when tightening test timing.
Other factors
The added comment satisfies the repo rule that a literal timing constant deviating from upstream needs a comment naming why. The expected-durations.json values are advisory and regenerated from CI periodically, so exact numbers don't need to be precise. The change follows the same precedent as #33622 (test-performance-eventloopdelay.js). No assertions, mustCall counts, or coverage were weakened — only the clock was compressed.
|
The modified test passed on every lane across both CI builds (#86302 and #86355). The remaining reds are unrelated parallel-batch flakes (napi, spawn-streaming-stdout, streams-leak, html-rewriter-leak, tls-namedpipes, etc.) that also pass when re-run alone; none touch the two files in this diff. Ready for review. |
|
Closing this PR because it has been inactive for more than 90 days. |
What
test/js/node/test/sequential/test-http-server-request-timeouts-mixed.jsis the upstream Node.jsheadersTimeout/requestTimeoutinterleave test. It was the slowest vendored sequential http test at ~12.5 s on the debian 13 x64-asan lane (build #86086).Cause
The test schedules every write and every assertion at a multiple of
headersTimeout(0.2x, 0.6x, 0.8x, 1.4x, 1.5x, 3.125x). Upstream sets the base tocommon.platformTimeout(2000), so the last callback fires at 3.125 * 2000 = 6.25 s on release lanes and 3.125 * 4000 = 12.5 s on debug builds whereprocess.features.debugmakesplatformTimeoutdouble its argument. On the CI asan lane (a release build withprocess.features.debug === false) the schedule is 6.25 s; the remaining ~6 s of the observed 12.5 s is LSAN /BUN_DESTRUCT_VM_ON_EXIT/validateExceptionChecksteardown the runner adds there.Change
Lower the base to
common.platformTimeout(1000). All delays scale with this value andconnectionsCheckingInterval = headersTimeout / 8scales with it too, so every relative margin is preserved. The narrowest window is request2's 408 arriving before the 1.4x checkpoint (0.075 * headersTimeout): 75 ms on every CI lane (all release, including asan), 150 ms on local debug builds.The 408 status line and the
common.mustCall(..., 4)/mustCallAtLeast/mustNotCallassertions are untouched, so coverage is the same as upstream; only the clock is compressed. Same pattern assequential/test-performance-eventloopdelay.js(#33622).test/expected-durations.jsonupdated so shard bin-packing reflects the new cost until the next regeneration. The asan entry only drops from 12509 to ~9384 because the schedule halves (6250 -> 3125) but the per-process LSAN/VM-destruct teardown does not.Verification
Local timing:
bun bd(debug+ASAN)./build/debug/bun-debugStability at
platformTimeout(1000):bun bd)*
BUN_JSC_validateExceptionChecks=1,BUN_JSC_dumpSimulatedThrows=1,BUN_DESTRUCT_VM_ON_EXIT=1,BUN_FEATURE_FLAG_NO_ORPHANS=1, LSAN enabled withtest/leaksan.supp; the exact envscripts/runner.node.mjssets forsequential/on the asan lane.Lower candidates were rejected: a fixed 800 ms base flakes 2/30 on the request2 window under debug+ASAN; 640 ms flakes 16/20.
no test proof · iteration 0 · docs-only change; test-proof not applicable